Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rollup #737

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Rollup #737

wants to merge 8 commits into from

Conversation

aminya
Copy link

@aminya aminya commented May 1, 2020

This is another proof of how and why the performance of atom-julia-client is suffering from the old JavaScriptsm and new wrong decisions.

Despite building, it throws all sorts of warnings and errors because of these wrong habits.

Circular dependencies are spread all over the codebase:

(!) Circular dependencies
lib\ui.coffee -> lib\ui\layout.js -> lib\runtime.coffee -> lib\runtime\evaluation.coffee -> lib\ui.coffee
lib\ui.coffee -> lib\ui\layout.js -> lib\runtime.coffee -> lib\runtime\evaluation.coffee -> lib\runtime\workspace.coffee -> lib\ui.coffee
lib\ui.coffee -> lib\ui\layout.js -> lib\runtime.coffee -> lib\runtime\evaluation.coffee -> lib\runtime\workspace.coffee -> C:\Users\yahyaaba\Documents\GitHub\JavaScript\github\julia-client\lib\ui.coffee?commonjs-proxy -> lib\ui.coffee
...and 7 more
created dist in 7.9s

Many unresolved dependencies:

lib/julia-client.coffee → dist...
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
child_process (imported by lib\misc\paths.js, lib\connection\terminal.coffee, child_process?commonjs-external)
net (imported by lib\connection\terminal.coffee, net?commonjs-external, lib\connection\process\basic.js, lib\connection\process\remote.js, lib\connection\process\tcp.coffee)
vm (imported by lib\runtime\frontend.coffee, vm?commonjs-external)
shell (imported by lib\package\commands.coffee, shell?commonjs-external)
remote (imported by lib\ui\notifications.coffee, remote?commonjs-external)

Many other dynamic requires that I tried to solve using plugins.

My suggestion for this repository:

  • Use named exports/imports
  • Stop using dynamic require.
  • Use dynamic import inside a condition (if)
  • Avoid circular dependencies by restructuring the modules, use proper abstraction, using self-contained classes.
  • Don't try to do everything in one package. Use services from other packages like linter, atom-ide packages, etc. Modularity is the best development approach.
  • deferring execution should be the last thing to try. The program should already run fast synchronized and then could benefit from asynchronous execution.
  • JSfying does not help if you want to follow the same old procedure.
  • Many tools exist that tell you these kinds of things (Eslint, TypeScript, etc). They exist for some reason.
  • and lastly, be open to suggestions. People want to help.

@pfitzseb
Copy link
Member

pfitzseb commented May 1, 2020

Can you explain what rollup is and why we should use it?

Anyways, I agree with most of your points. We definitely should restructure code at the same time as translating it form CoffeeScript to JS, which is precisely why automating that task is only somewhat helpful.

Circular dependencies are spread all over the codebase:

Yeah, that's pretty bad...

Many unresolved dependencies:

Eh, those are whatever -- in theory we shouldn't rely on those being in the global context (just like the atom global, I suppose), but in practise that really doesn't matter much.

Use named exports/imports
Stop using dynamic require.

👍

Don't try to do everything in one package. Use services from other packages like linter, atom-ide packages, etc. Modularity is the best development approach.

Eh, not so sure about that. Most Atom packages aren't that great to interface with (and/or provide an awful UX). atom-linter is probably the exception there, and we should use it once we actually have a linter (and for the Traceur integration, I suppose).

deferring execution should be the last thing to try. The program should already run fast synchronized and then could benefit from asynchronous execution.

Hm, not sure what you mean here. I think we should make sure to (re-) design everything with lazy-loading and as much asynchronicity in mind -- it's pretty hard to shoehorn that in afterwards without making a mess of everything.

and lastly, be open to suggestions. People want to help.

👍
But also appreciate that we don't have an unlimited amount of time to work on this (and even reviewing PRs takes a lot of time when they touch huge parts of the codebase). I also think performance isn't the biggest problem Juno has right now.

@aminya
Copy link
Author

aminya commented May 2, 2020

Can you explain what rollup is and why we should use it?

It is a bundler similar to Webpack, but with better plugins. It reduced the load time (even now that the code has many issues) to half of its time. So once the Junos requires are converted to import,export, you can get a huge bump in the performance.

2020-05-01 05_44_00-Settings — C__Users_yahyaaba_Documents_GitHub_JavaScript_github_julia-client — A

2020-05-01 05_44_37-Settings — C__Users_yahyaaba_Documents_GitHub_JavaScript_github_julia-client — A

Don't try to do everything in one package. Use services from other packages like linter, atom-ide packages, etc. Modularity is the best development approach.

Eh, not so sure about that. Most Atom packages aren't that great to interface with (and/or provide an awful UX). atom-linter is probably the exception there, and we should use it once we actually have a linter (and for the Traceur integration, I suppose).

atom-ide-community packages are very well. linter is very good too. If there is something you don't like about them, you should improve those packages instead of writing everything from scratch here.

deferring execution should be the last thing to try. The program should already run fast synchronized and then could benefit from asynchronous execution.

Hm, not sure what you mean here. I think we should make sure to (re-) design everything with lazy-loading and as much asynchronicity in mind -- it's pretty hard to shoehorn that in afterwards without making a mess of everything.

Here I meant relying on deferring execution isn't good when the codebase already has other issues. It should be tried after.

and lastly, be open to suggestions. People want to help.

👍
But also appreciate that we don't have an unlimited amount of time to work on this (and even reviewing PRs takes a lot of time when they touch huge parts of the codebase).

That is why you opened the source of the packages. People will put time on helping the package improve.

I also think performance isn't the biggest problem Juno has right now.
Writing good code is always important, even if you don't care about performance. The things I mentioned are not that complicated, most of them are a matter of just replacing a few lines of code.

Regarding the Atom's performance, people complain about it all over the internet. Many (including the actual Atom developers) have already switched to other platforms! We should all write performant code, especially when we gain a lot with very minor adjustments (like import and require).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants